-
Notifications
You must be signed in to change notification settings - Fork 296
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Exporter.Geneva] Add named options support for GenevaTraceExporter and GenevaMetricExporter #1218
[Exporter.Geneva] Add named options support for GenevaTraceExporter and GenevaMetricExporter #1218
Conversation
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #1218 +/- ##
==========================================
+ Coverage 73.17% 74.07% +0.90%
==========================================
Files 260 265 +5
Lines 9133 9347 +214
==========================================
+ Hits 6683 6924 +241
+ Misses 2450 2423 -27
|
|
||
if (configure != null) | ||
{ | ||
builder.ConfigureServices(services => services.Configure(name, configure)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FYI this will work fine today for metrics & traces because they have different options classes. Logging appears to be sharing GenevaExporterOptions
with tracing so if/when logging gets hooked up to DI/options there will be issues with the delegates potentially clobbering each other. Cross that bridge when we come to it 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Changes
GenevaTraceExporter
andGenevaMetricExporter
AddGenevaMetricExporter
without any parameters to avoid warning RS0026Please provide a brief description of the changes here.
For significant contributions please make sure you have completed the following items:
CHANGELOG.md
updated for non-trivial changes